Add ReadAnalogWaveforms to NiDAQmxService #1203
Conversation
- Added ReadAnalogWaveforms method stub to NiDAQmxService for reading analog waveforms. - Updated metadata validation to include CustomCodeNoLibrary. - Introduced new waveform attributes and functions in metadata. - Enhanced CMake configuration for new protobuf files. - Improved CONTRIBUTING.md with Ninja build instructions.
mikeprosserni
changed the title
mikeprosserni
requested review from
astarche,
maxxboehme and
reckenro
as code owners
.github/workflows/build_cmake.yml
Outdated
| glibc_version: "2_31", | ||
| cmake_generator: '-G "Ninja"', | ||
| build_type: RelWithDebInfo | ||
| build_type: RelWithDebInfo, |
There was a problem hiding this comment.
I made these build config changes in order to get nidaqmx_library.cpp to successfully compile on Ubuntu. As far as I can tell, the problem is that after I added the new library functions in functions.py, the nidaqmx_library.cpp became too large to compile with the original settings. It was either timing out or running out of memory or something.
I'm not sure this is the best way to fix the issue, however. Changing the optimization from O2 to O1 means the grpc device server will be less optimized for everyone on linux, right? Another approach that might work is splitting nidaqmx_library.cpp into multiple smaller .cpp files somehow. What do the reviewers think of that?
There was a problem hiding this comment.
When you build it locally, does it print any more output, such as
note: variable tracking size limit exceeded with -fvar-tracking-assignments, retrying without
In the past I've sometimes had to use -fno-var-tracking-assignments to compile large code-generated files. I don't know if this option is still relevant or there is a different option nowadays.
There was a problem hiding this comment.
Also, how long does it take to build locally with -O2? Does it eventually succeed?
There was a problem hiding this comment.
I don't have a linux machine to build locally on, I've been relying on the CI build for this. All I can see is this, after 33 minutes:
[2601/3669] Building CXX object CMakeFiles/IntegrationTestsRunner.dir/generated/nidaqmx/nidaqmx_library.cpp.o
ninja: build stopped: interrupted by user.
Error: Process completed with exit code 143.
https://github.com/ni/grpc-device/actions/runs/18659129011/job/53195445860
or, when I added --verbose:
[2601/3669] /usr/bin/g++-9 -DCARES_STATICLIB -DTEST_API_BUILDING -DTestApi -D_CRT_SECURE_NO_WARNINGS -D_WINSOCK_DEPRECATED_NO_WARNINGS -Iproto -I.././generated -I.././imports/include -I.././source -I.././third_party/grpc-sideband/src -I.././third_party/grpc-sideband/moniker_src -I../third_party/grpc/include -I../third_party/grpc/third_party/abseil-cpp -I../third_party/grpc/third_party/re2 -Igrpc/third_party/cares/cares -I../third_party/grpc/third_party/cares/cares -I../third_party/grpc/third_party/cares/cares/include -I../third_party/grpc/third_party/boringssl-with-bazel/src/include -I../third_party/grpc/third_party/protobuf/src -I../third_party/utfcpp/source -I../third_party/grpc-sideband/src -I../third_party/grpc-sideband/moniker_src -I../third_party/json/include -isystem ../third_party/googletest/googlemock/include -isystem ../third_party/googletest/googlemock -isystem ../third_party/googletest/googletest/include -isystem ../third_party/googletest/googletest -O2 -g -DNDEBUG -std=c++17 -MD -MT CMakeFiles/IntegrationTestsRunner.dir/generated/nidaqmx/nidaqmx_library.cpp.o -MF CMakeFiles/IntegrationTestsRunner.dir/generated/nidaqmx/nidaqmx_library.cpp.o.d -o CMakeFiles/IntegrationTestsRunner.dir/generated/nidaqmx/nidaqmx_library.cpp.o -c ../generated/nidaqmx/nidaqmx_library.cpp
ninja: build stopped: interrupted by user.
Error: Process completed with exit code 143.
https://github.com/ni/grpc-device/actions/runs/18663525332/job/53209586512
I did some googling and some guessing, and figured resource exhaustion was the most likely explanation for exit code 143. So I changed the build parameters to see if that would get the CI to build successfully, and it did. That's all I know at this point.
There was a problem hiding this comment.
VTA is a potential cause of resource exhaustion. This feature tracks variable locations for each line of code in order to produce debug info that accurately shows the variable values when you step through the function, even if it's inlined. However, when your code has hundreds of variable assignments in the same function, this feature can take a lot of CPU and memory. I don't know exactly why.
VTA only affects the debugging experience, not execution speed, so if turning it off solves your problem, then it's a better solution than using -O1 and turning off a bunch of optimizations.
Please try updating https://github.com/ni/grpc-device/blob/main/source/codegen/templates/service.cpp.mako to use #pragma GCC optimize to scope the -fno-var-tracking-assignments or -O1 to *_library.cpp. You will need to guard this pragma with #ifdef __GNUC__ or similar.
There was a problem hiding this comment.
In this PR, I've reverted the changes to build_cmake and implemented your suggested changes to service.cpp.mako, and nidaqmx_library.cpp is still compiling without crashing.
As another data point, I tried a different strategy in a different PR, where I reverted the other changes in build_cmake, and just used -j ${{ runner.os == 'Linux' && '2' || steps.cpu-cores.outputs.count }}, which also allwed nidaqmx_library.cpp to compile without crashing.
There was a problem hiding this comment.
I checked out your branch on my laptop, commented out the -fno-var-tracking-assignments, and ran time cmake --build . -j 16 --config RelWithDebInfo on WSL:
real 21m28.891s
user 298m4.721s
sys 30m30.097s
Running .ninja_log through https://github.com/nico/ninjatracing shows
It seems like all of the DAQmx files are problem files, taking >3 minutes to compile.
There was a problem hiding this comment.
Wait, which file is causing the problem? service.cpp or library.cpp? I thought you said it was library.cpp.
There was a problem hiding this comment.
nidaqmx_library.cpp was the problem. Although the changes made in service.cpp.mako seems to have fixed it: https://github.com/ni/grpc-device/actions/runs/18724889717/job/53406944072?pr=1203
[2600/3669] Building CXX object CMakeFiles/IntegrationTestsRunner.dir/proto/data_moniker.grpc.pb.cc.o
[2601/3669] Building CXX object CMakeFiles/IntegrationTestsRunner.dir/generated/nidaqmx/nidaqmx_library.cpp.o
[2602/3669] Building CXX object CMakeFiles/IntegrationTestsRunner.dir/generated/nidaqmx/nidaqmx_service_registrar.cpp.o
There was a problem hiding this comment.
In my other PR, I'm trying out those changes in library.cpp.mako instead of service.cpp.mako: https://github.com/ni/grpc-device/pull/1205/files#diff-0cb12eeb765640c93fc98cdc6777d834a23c8042f953a4e41186bc87b72fa252
https://github.com/ni/grpc-device/actions/runs/18726362277/job/53412058096?pr=1205
(note, this did not work, the build failed)
There was a problem hiding this comment.
That's weird. NiDAQmxLibrary::NiDAQmxLibrary(std::shared_ptr<nidevice_grpc::SharedLibraryInterface> shared_library) is only called from NiDAQmxLibrary::NiDAQmxLibrary() and doesn't seem to be inlined in the v2.13.0 release binaries.
That said, the build time increased by 20 minutes, so I think there may be multiple problem files. nidaqmx_library.cpp may have been the one that pushed the GitHub-hosted runner over the edge and made it run out of memory, but other files may be taking several more minutes than before.
If disabling VTA fixes the build, then I think it's worth seeing what happens when you disable VTA globally instead of per-file.
There was a problem hiding this comment.
That said, the build time increased by 20 minutes, so I think there may be multiple problem files. nidaqmx_library.cpp may have been the one that pushed the GitHub-hosted runner over the edge and made it run out of memory, but other files may be taking several more minutes than before.
Oops, I was looking at the -j 2 change. Yeah, that's going to slow down the build.
There was a problem hiding this comment.
Should I still try disabling VTA globally?
There was a problem hiding this comment.
Ok, I've updated this PR to disable VTA globally. (since the changes with push/pop didn't work)
I reverted all other build changes, and added this to CMakeLists.txt:
# GCC-specific flags
if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-var-tracking-assignments")
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fno-var-tracking-assignments")
endif()
(Note: This worked, the Ubuntu build succeeded after 66 minutes)
| get_filename_component(debugsessionproperties_restricted_proto "source/protobuf_restricted/debugsessionproperties_restricted.proto" ABSOLUTE) | ||
| get_filename_component(calibrationoperations_restricted_proto "source/protobuf_restricted/calibrationoperations_restricted.proto" ABSOLUTE) | ||
| get_filename_component(data_moniker_proto "imports/protobuf/data_moniker.proto" ABSOLUTE) | ||
| get_filename_component(precision_timestamp_proto "third_party/ni-apis/ni/protobuf/types/precision_timestamp.proto" ABSOLUTE) |
There was a problem hiding this comment.
Similar to your last PR, I think you'll need to update stage_client_files.py to get these proto files included in GitHub releases and the AzDo consumed adhoc exports.
|
|
||
| message ReadAnalogWaveformsRequest { | ||
| nidevice_grpc.Session task = 1; | ||
| int32 number_of_samples_per_channel = 2; |
There was a problem hiding this comment.
ReadAnalogF64 calls this num_samps_per_chan. Nothing in nidaqmx.proto spells out "number_of_samples".
| } | ||
|
|
||
| if (num_channels == 0) { | ||
| return ::grpc::Status(::grpc::INVALID_ARGUMENT, "No channels found in task"); |
There was a problem hiding this comment.
READ_ATTRIBUTE_NUM_CHANS isn't the number of channels in the task, it's the number of channels specified by DAQmx_Read_ChannelsToRead.
| for (uInt32 i = 0; i < num_channels; ++i) { | ||
| response->add_waveforms(); | ||
| } |
There was a problem hiding this comment.
Call response->mutable_waveforms().Reserve(num_channels) to grow the RepeatedPtrField to the exact size rather than doubling when needed.
| // Callback data structure to pass waveform pointers to the callback | ||
| struct WaveformCallbackData { | ||
| std::vector<::ni::protobuf::types::DoubleAnalogWaveform*> waveforms; | ||
| }; |
There was a problem hiding this comment.
I think you can replace this struct with RepeatedPtrField<DoubleAnalogWaveform>. It's already an STL-style container.
Also, when you add DigitalWaveform support, I think you can template this function on the waveform type.
| y_data->Reserve(samples_per_chan_read); | ||
| for (int32 j = 0; j < samples_per_chan_read; ++j) { | ||
| y_data->Add(read_arrays[i][j]); | ||
| } |
There was a problem hiding this comment.
Use the iterator version of Add. It's probably optimized to use memmove().
y_data->Add(read_arrays[i], read_arrays[i] + samples_per_chan_read);
If MSVC complains about using unsafe iterators, there are extensions like https://learn.microsoft.com/en-us/cpp/standard-library/checked-array-iterator-class?view=msvc-170 which you can use in #ifdef _MSC_VER or whatever.
| // Convert from 100ns ticks (DAQmx format) to PrecisionTimestamp | ||
| // t0_array[i] contains 100ns ticks since Jan 1, 0001 (.NET DateTime epoch) | ||
| const int64_t seconds = t0_array[i] / TICKS_PER_SECOND; | ||
| const int64_t fractional_ticks = t0_array[i] % TICKS_PER_SECOND; | ||
|
|
||
| t0->set_seconds(seconds); | ||
| t0->set_fractional_seconds(fractional_ticks); |
There was a problem hiding this comment.
These fractional ticks are in the wrong units (.NET 100 ns ticks vs. NI-BTF 1/2^64 second ticks).
Look at how TimeSpanToCVITime does it in the .NET API. It calculates the fractional seconds as a double, then multiplies the double by ulong.MaxValue to convert it to NI-BTF ticks.
Please extract this conversion into a helper function and write a unit test that converts times using well-known fractions like 1/4, 1/2, and 3/4. Example: https://github.com/ni/nidaqmx-python/blob/fa2a2bbf0b95ac25350542f1362e3e23cc43bcb0/tests/unit/test_lib_time.py#L166
| return create_ai_voltage_chan(request, response); | ||
| } | ||
|
|
||
| ::grpc::Status create_two_ai_voltage_chans(double min_val, double max_val, CreateAIVoltageChanResponse& response = ThrowawayResponse<CreateAIVoltageChanResponse>::response()) |
There was a problem hiding this comment.
This seems overly specific, better make it create_n_ai_voltage_chans so the next guy doesn't write a create_three_ai_voltage_chans.
I'm kind of surprised create_ai_voltage_chan doesn't let you specify the name, but whatever.
| EXPECT_THAT(actual, ContainerEq(COEFFICIENTS)); | ||
| } | ||
|
|
||
| TEST_F(NiDAQmxDriverApiTests, ReadAnalogWaveforms_WithNoAttributeMode_ReturnsWaveformData) |
There was a problem hiding this comment.
This should probably validate that timing and attributes are unset.
| auto now_since_year1 = now + epoch_offset_year1_to_1970; | ||
| const auto& timestamp = waveform.t0(); | ||
| EXPECT_NEAR(timestamp.seconds(), now_since_year1, 1); | ||
| EXPECT_NE(timestamp.fractional_seconds(), 0); |
There was a problem hiding this comment.
Random timestamps have fractional seconds of 0, sometimes.
| // Get current time in seconds since year 1 AD (Jan 1, 0001) - .NET DateTime epoch | ||
| // This matches the format used by DAQmxInternalReadAnalogWaveformPerChan | ||
| // From year 1 AD to 1970-01-01 is 719162 days * 24 * 3600 = 62135596800 seconds | ||
| const auto epoch_offset_year1_to_1970 = 62135596800LL; | ||
| auto now = std::chrono::duration_cast<std::chrono::seconds>( | ||
| std::chrono::system_clock::now().time_since_epoch()).count(); | ||
| auto now_since_year1 = now + epoch_offset_year1_to_1970; | ||
| const auto& timestamp = waveform.t0(); |
3 participants
What does this Pull Request accomplish?
library_->InternalReadAnalogWaveformPerChan(), which has very different parameters)Why should this Pull Request be merged?
AB#3424627
What testing has been done?